The new history file path is:

echo "${XDG_DATA_HOME:-$HOME/.local/share}/swarm/history"

The quick and easy way to migrate old history using ghci:

data R = REPLEntry Bool String deriving Read
new_history = "<output of shell command above>"
readFile ".swarm_history" >>= pure . unlines . map (\(REPLEntry _ t) -> t) . read >>= writeFile new_history

Weird, I re-run the failing tests and have no idea what their problem is 😕 Maybe it will go away once rebased on main.
Oh, now I see #252 it makes sense 😄

I am unhapy with the current REPLEntry Bool Bool situation - I am considering:

• splitting it into a separate type - data REPLHistItem = REPLIn REPLEntry | REPLOut, with lenses on REPLEntry
  • this would make it easier to add other properties
• wrapping history, using some other container and having the neighbor logic in some function
  • I guess we could get by with just an index to start of loaded history and avoid Bools altogether

I could also do it as separate PR/Issue, but the first option is easy enough to do here.
What do you think @byorgey, @Alexander-Block? 🙂

I also agree that REPLEntry Bool Bool is far from optimal; upon reflection it mainly bothers me that the semantics of each Bool is not immediately obvious without reading the haddock comment for REPLHistItem. Apart from that I think that marking duplicates with an attribute is not a bad solution. This would be solved by your first proposed fix.

Do you see concrete advantages of your second proposal compared to the first one, @xsebek? If not, I slightly prefer the first one for its simplicity. I guess it should not be too hard to refactor to the second proposal later if it turns out to be a better solution.

However, this is also not a particularly strong opinion.

@Alexander-Block I thought about it and I think wrapping Seq is the right way ™️.

We are indexing a lot into history and Seq has some nice functions to find first index matching predicate from both directions, so that makes it superior in my eyes. There is some added complexity in adding new function names to remember but on the other hand Seq could be more appealing to imperative programmers that like random access.

I somehow managed to change it and get it to work again (off by one errors are the worst) but it could definitely be refactored further. I originally hoped to have all functions taking Seq REPLHistItem in Model.hs, but maybe it is not worth the trouble.

EDIT: just to clarify, with sequence I abandoned stack-like indexing so the historically first entry has index 0 and instead of -1 I use replLength for index of current buffer. I first tried keeping the old indices, but it did not play nicely with findIndexR.

Thank you for this thorough explanation. I agree with your arguments both for using Seq for the REPL history as well as wrapping it.

However, I also agree that this implementation could benefit from further refactoring. I find it a bit worrying to expose details of the inner storage outside Model.hs because it partially defeats the advantages of wrapping the REPL history by making clients dependent on an implementation detail. If we furthermore concentrate the logic for manipulating and accessing the REPL history in Model.hs we get the additional benefit of improving testability. Since you already mention that you had to deal with off-by-one errors, I think that it would be a good idea to add a suite of unit tests to make sure that the new implementation of the REPL history conforms (and continues to conform) to our expectations.

I would be happy to go through your code changes an do a more detailled review; I did not find the time to do that yesterday (which is why I write this vague comment), but I think that I will find time at some point today (from the perspective of the CEST time zone). Also I would be happy to propose unit tests as it will help me to better understand your code changes.

Thanks for your detailed review @Alexander-Block, I am working through it and fixing bugs! 🐛🔨

For fun I also added a debugging line to show the index:

┌──────────────── string ──┐
│ > (\x. "a") 1            │
│ 56                       │
└──────────────────────────┘

It is triggered by this line in drawREPL:

debugging = False -- Turn ON to get extra line with history index
inputLines = 1 + fromEnum debugging

Could be useful for #134. 🙂

I'm pretty happy with the result and I even added some unit tests so hopefully it works now. 😄

Could you please check it out and see if I did not miss anything, @Alexander-Block and @byorgey?

I noticed that the status of my review was still "requesting changes"; the reason for this status was the bug I noticed and which has since been fixed. There are still unresolved conversations, but these contain only comments which may or may not lead to changes in this PR and should probably not prevent a merge.

I think it looks good too, though I seem to recall @xsebek wanting to add some tests first? Or am I getting it confused with something else?

@xsebek has added unit tests (cf testModel in Unit.hs). They look fine to me, although I didn't explicitly commented on them.

@byorgey You might be thinking about #270, where I would indeed like to write some tests first 🙂

@Alexander-Block sorry for the delay, I actually had the changes you proposed implemented for almost a week now, just did not get around to test and push them 😉